Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Asset context overview page. #1369

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Muhammad-Moiz626
Copy link
Contributor

@Muhammad-Moiz626 Muhammad-Moiz626 commented Mar 12, 2025

Description

Summary of the changes introduced in this PR. Try to use bullet points as much as possible.

  • ...
  • Created a new page for Asset context overview

Look & Feel

image

How to test

To test just go to Assets listing page and click on any asset to see a high level diagramatic view of the Assets.

Further Improvements

Adding new features like listing the Asset sensors in the Asset Node.

Related Items

Closes #1350


  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

treeSpecs,
{renderer: "svg"}
);
function openModal(data) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome direction you're taking this in! It looks very clean to have the sensor listing in the modal..

image

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! At this time, I still see this as a PoC, but it's promising. My comments deal mostly with the tree.

On my side, I have to figure out how to make this page look okay if there is just one asset. You seem to be experimenting with the simulation structure, which is naturally rich, but in the EMS, people might often (or at first) just have one or two assets.

First, there are some display issues with the tree:
For me, on Firefox, there is horizontal jiggling when I hover the tree. On both Firefox and Chrome, the initial size is smaller than it is after I hovered for the first time.

Here is a video made on Firefox:

vokoscreenNG-2025-03-17_12-39-09.webm

Second, the tree also is not responding well to mobile view. And I don't expect we can make it. (apart from making the tree vertical, which would help somewhat). To make it easy for ourselves, we could have a div with a simple text view (name the parent asset, list children and provide the button to create a child) and show that when the screen becomes narrow (in flexmeasures.css we already use these selectors, e.g. @media (max-width: 767px) {})

Other than the tree, there are two issues:

  • I want to simplify the map, I believe you don't need a new view and there is less things to include.
  • The link to create a child should become smarter. This might mean to make the view smarter.

See my inline comments.

<div class="col-sm-2"></div>

<!-- Bootstrap Modal -->
<div class="modal fade" id="nodeModal" tabindex="-1" aria-labelledby="modalTitle" aria-hidden="true">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ID should be something like sensorsModal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not resolved yet

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a complete review, just about the new asset improvement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I press the existing new asset button (on the asset page) I get

"local variable 'parent_asset_name' referenced before assignment"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I press the existing new asset button (on the asset page) I get

"local variable 'parent_asset_name' referenced before assignment"

I wasn't able to reproduce this one. Can you please test again?

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why, but the map is not showing for me, even though the dashboard map does work.

Screenshot from 2025-03-19 23-48-22

<div class="col-sm-2"></div>

<!-- Bootstrap Modal -->
<div class="modal fade" id="nodeModal" tabindex="-1" aria-labelledby="modalTitle" aria-hidden="true">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not resolved yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset context overview page
3 participants